feat(tanstackstart-react): Add file exclude to configure middleware auto-instrumentation#19007
feat(tanstackstart-react): Add file exclude to configure middleware auto-instrumentation#19007nicohrubec wants to merge 31 commits intodevelopfrom
Conversation
This reverts commit 330d7f8.
2ca2145 to
e0df795
Compare
e0df795 to
e1276d9
Compare
e1276d9 to
864d5b0
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
…uto-instrumentation
86f5879 to
07f4f30
Compare
Codecov Results 📊Generated by Codecov Action |
| import type { Plugin } from 'vite'; | ||
|
|
||
| type AutoInstrumentMiddlewareOptions = { | ||
| enabled?: boolean; |
There was a problem hiding this comment.
This was an unnecessary option since we just don't add this plugin if it's not enabled, so I removed it
| transform(code, id) { | ||
| if (!enabled) { | ||
| // Skip if not a TS/JS file | ||
| if (!/\.(ts|tsx|js|jsx|mjs|mts)$/.test(id)) { |
There was a problem hiding this comment.
l: This was here before - still, maybe it makes more sense to have a path.extname(id) instead and check against this, it might be faster than regex or simply an endsWith (might be more effort, since this one only takes a string)
| // file matches exclude patterns, skip | ||
| if (debug) { | ||
| // eslint-disable-next-line no-console | ||
| console.log(`[Sentry] Skipping auto-instrumentation for excluded file: ${id}`); |
There was a problem hiding this comment.
l: I think this should be rather debug.log, where debug comes from @sentry/core
There was a problem hiding this comment.
I think this needs to be console.log since it happens during build not runtime
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| type AutoInstrumentMiddlewareOptions = { | ||
| enabled?: boolean; | ||
| debug?: boolean; | ||
| exclude?: Array<string | RegExp>; |
There was a problem hiding this comment.
Maybe it would make sense to add some defaults here. E.g. excluding the test files if this is a very common usecase
There was a problem hiding this comment.
I'll think about this some more and maybe do a follow up, for the test files for instance I think the better approach for us would be to add this to the skipped file extensions early return instead of using this option
|
Update: So the current state is what I would like this to look like, meaning only patching the original source files so users can reasonably exclude certain files. However, for some reason excluding js files from the transformations results in a slight regression where auto-instrumentation for non-global function middleware is not working anymore. I haven't found an idea yet that would make this work for function middleware. So thinking about closing this. If users want to opt-out they still can be simply disabling middleware auto-instrumentation fully. |
Depends on (i.e. do not review until the following PR is merged): #19001
This PR extends the autoInstrumentMiddleware option to support fine-grained control over which files are auto-instrumented. Previously, this option only supported a boolean setting. Now you can pass an object with an exclude array to selectively skip specific files from auto-instrumentation.
I also tightened the auto-instrumented file extension to only typescript files. Tanstack start recommends to use Typescript anyways so this should cover most user applications and including js files leads to tempering with post-transpiled files, which I don't really like. This way we patch the original source and that's it.
Closes #18985